Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manage GraphBLAS version with GB_VERSION.txt file #114

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

alugowski
Copy link
Collaborator

This is a bit out of left field and might not be necessary, but food for thought. Feel free to just close.

Add GB_VERSION.txt that contains the version of GraphBLAS to build against.

As far as I can tell this is currently only specified in the git tag. That works well for tagged commits like releases, but sometimes introduces issues for development commits. I've done a lot of dev with extra commits to hard-code refs since the automatic methods would result in v0.0.1.

This restores the DRY principle by having the places that need the GraphBLAS version number to read it from the file. If anything this at least makes upgrades a little bit easier.

Also adds:

The tag method is still kept for releases to ensure no accidental version mismatches on release.

Also include a script to fetch latest upstream GraphBLAS version, as well as a workflow that can auto-update this file on a schedule.
@eriknw
Copy link
Member

eriknw commented Oct 17, 2023

LGTM! I understand the limitations of the previous approach, and I would also occasionally hard-code versions during development.

One thing... I would add a shebang line in latest_suitesparse_graphblas_version.py and make sure it's executable:

#!/usr/bin/env python

@eriknw eriknw self-requested a review October 17, 2023 14:14
@alugowski
Copy link
Collaborator Author

One thing... I would add a shebang line in latest_suitesparse_graphblas_version.py and make sure it's executable:

#!/usr/bin/env python

Good point. Added.

@alugowski
Copy link
Collaborator Author

Food for thought: If the version stuff becomes an issue in the future it might be worth considering this pattern: https://setuptools-git-versioning.readthedocs.io/en/stable/schemas/file/dev_release_file.html

I think all that would be required is to convert GB_VERSION from a file pointing at a GraphBLAS version to one pointing at a python-suitesparse-graphblas version, i.e. the same thing but with a ".0" (or other integer) suffix. The scripting for that would be pretty simple.

A bit overkill at the moment though.

@alugowski
Copy link
Collaborator Author

Also note that the workflow in this PR is probably overkill. Its greatest benefit is to serve as documentation for what parts of the repo need to be modified for each update. I mostly wrote and tested it as a sort of devops practice.

Since GraphBLAS updates are not too common, the manual method might be even easier: use GitHub's edit button on GB_VERSION.txt and let GitHub create the PR.

@eriknw
Copy link
Member

eriknw commented Oct 19, 2023

Its greatest benefit is to serve as documentation for what parts of the repo need to be modified for each update.

heh, good to know... and I think this is a great benefit!

Since GraphBLAS updates are not too common, the manual method might be even easier: use GitHub's edit button on GB_VERSION.txt and let GitHub create the PR.

Yeah, manually specifiying the GraphBLAS version and choosing the correct tag version hasn't really been a pain point, so I'm fine keeping this simple and manual for now. This PR does solve actual pain points.

Question: can we make this work with SuiteSparse:GraphBLAS beta releases such as 8.2.1.beta3? It's nice to be able to test out his betas, and maybe we could actually publish pre-releases using his betas too. I can test this out in a day or two--I think maybe we can add a check to suitesparse.sh to specifically handle betas.

Anyway, I think this PR is good to merge. If I find any issues when I get around to playing with it in a day or two, I'll let you know!

@alugowski
Copy link
Collaborator Author

Question: can we make this work with SuiteSparse:GraphBLAS beta releases such as 8.2.1.beta3? It's nice to be able to test out his betas, and maybe we could actually publish pre-releases using his betas too. I can test this out in a day or two--I think maybe we can add a check to suitesparse.sh to specifically handle betas.

A quick glance suggests the main sticking point would be the version regex in suitesparse.sh, which only pulls out the first three digits (i.e. 8.2.1.beta3 would result in 8.2.1):

if [[ $1 =~ refs/tags/([0-9]*\.[0-9]*\.[0-9]*)\..*$ ]]; then
    VERSION=${BASH_REMATCH[1]}

Other than that, I'm not sure how the python version logic and PyPI would handle a fifth dot field. Upload to Test PyPI to verify.

Anyway, I think this PR is good to merge. If I find any issues when I get around to playing with it in a day or two, I'll let you know!

🎉

@alugowski alugowski merged commit 70fe204 into GraphBLAS:main Nov 1, 2023
24 checks passed
@alugowski alugowski deleted the gbversion branch December 16, 2023 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants